Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dapr component schema #460

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

FullStackChef
Copy link
Contributor

@FullStackChef FullStackChef commented Feb 9, 2025

**Closes #459 & #507 **

PR Checklist

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • New integration
    • Docs are written
    • Added description of major feature to project description for NuGet package (4000 total character limit, so don't push entire description over that)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Contains NO breaking changes
  • [] Every new API (including internal ones) has full XML docs
  • Code follows all style conventions

Other information

@FullStackChef FullStackChef requested review from aaronpowell and removed request for WhitWaldo and paule96 February 9, 2025 07:55
return await contentWriter(content).ConfigureAwait(false);
}

private string GetAppHostRelativePath(string componentName)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside, we're creating a place to store things like this in 9.1, the aspire store.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice -Apphost relative? wanna point me to the code and I could update inline with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look at the aspire store interface - but it looks like a place to write data to rather than a location for user defined data - the idea of the app relative path is to provide a consistent location for users to place dapr components relative to the project rather than in the userprofile directory

@paule96
Copy link

paule96 commented Feb 10, 2025

Maybe if it is not to big of a no scope of this PR we should already start deviding dapr sidecar and component lifecycles to make our lifes easier later.

So that we would have two LifecycleHooks and that everytime you have something like WithReference on a sidecare you wait for this resource to complete.

With this we will have an easier life if we need to consider the endpoints to generate the correct YAML files

@FullStackChef
Copy link
Contributor Author

@paule96 my preference would be not to increase the scope of the my PR and I think the concept of separating lifecycle hooks needs further thought /design

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this file still? I don't see the reference of it in the sample

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file is used by this line of the apphost

var pubSub = builder.AddDaprPubSub("pubsub")

this is not new - it's simply in a project relative location

// If any of the components have secrets, we will add an on-demand secret store component.
if (onDemandComponents.Any(component => component.TryGetAnnotationsOfType<DaprComponentSecretAnnotation>(out var annotations) && annotations.Any()))
{
onDemandComponents.Add(new DaprComponentResource("secretstore", DaprConstants.BuildingBlocks.SecretStore));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this really a good idea? I mean a secret store can / should have configuration.
To just generate a generic one sounds a bit confusing to me.
Maybe we should better throw an exception here, that the user forgot something to define.
If we find secrets but no secret store

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's that simple. As a developer when I'm working locally, I want to be able to reference aspire parameters in my dapr metadata. If I have set those parameters as secrets then a dapr integration should respect that and treat them as such without me having to specifically set up a component.

Especially because when my resource requires secrets aspire automatically adds these secrets to a key vault and then references the key vault

However - looking at my implementation - I have overlooked what should happen if the user has configured a secrets component which should take priority

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at this again and it turns out I did consider a user defined secrets component - like the existing pubsub & state, the secretstore will infact only use the local env in the absence of a secretstore.yaml in the apphost relative component directory (apphost/.dapr/components) and the dapr default component directory (userprofile/.dapr/components)

Comment on lines 462 to 468
string componentPath = await (component.Type switch
{
DaprConstants.BuildingBlocks.PubSub => GetPubSubAsync(component, contentWriter, cancellationToken),
DaprConstants.BuildingBlocks.StateStore => GetStateStoreAsync(component, contentWriter, cancellationToken),
_ => throw new InvalidOperationException($"Unsupported Dapr component type '{component.Type}'.")
DaprConstants.BuildingBlocks.PubSub => GetBuildingBlockComponentAsync(component, contentWriter, "pubsub.in-memory", cancellationToken), // NOTE: In memory component can only be used within a single Dapr application.
DaprConstants.BuildingBlocks.StateStore => GetBuildingBlockComponentAsync(component, contentWriter, "state.in-memory", cancellationToken),
DaprConstants.BuildingBlocks.SecretStore => GetBuildingBlockComponentAsync(component, contentWriter, "secretstores.local.env", cancellationToken),
_ => GetComponentAsync(component, contentWriter, cancellationToken)
}).ConfigureAwait(false);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why their is still a need for this switch / case?

Shouldn't we handle everything with the last line? (GetComponentAsync(component, contentWriter, cancellationToken))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because the first three cases in the switch statement handle specific building blocks with local development defaults

Comment on lines +23 to 24
.WithReference(pubSub)
.WithDaprSidecar()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a developer standpoint I would more expect something like:

.WithDaprSidecar()
  .WithReference(pubSub)

Because you configure the sidecare to use the pubSub reference as a pubsub. Not the project.
That would also mean that we would need to break with the Fluent API their or have some WithDaprSidecar parameter that accepts a action or so to configure the sidecar.

You can look for the AzurePostgreSql resource of aspire how you can configure the development contianer. I would prefer an API like their

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent breaking changes for now closing this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is up to the developer the both work the same

@github-actions github-actions bot added the Stale label Feb 19, 2025
@aaronpowell
Copy link
Member

/stale-extend

@aaronpowell
Copy link
Member

/stale-extend

@aaronpowell aaronpowell removed the Stale label Feb 26, 2025
@FullStackChef
Copy link
Contributor Author

Quickly added parent relationship to address #507
image

@aaronpowell
Copy link
Member

@FullStackChef is this ready to be considered reviewable/mergable?

@FullStackChef
Copy link
Contributor Author

From my point of view, yes

Copy link
Member

@aaronpowell aaronpowell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a purely code perspective, it looks all good, but as I'm not across this problem space I'll delegate the "does it work" stuff to others.

But if there are no complaints over the next few days I'd take that as "ready to merge"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define Dapr Component Schema Model & WithMetadata Extension Method
4 participants